Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

report: restore old, disabled failed grouping test #7006

Merged
merged 2 commits into from
Jan 14, 2019
Merged

Conversation

brendankenny
Copy link
Member

This was an old test we made the test runner skip because of changing architecture at one point, but it turns out we don't really have another test of the grouping of failing audits, which is kind of important. This restores the test :)

TMI:
I mentioned it in #5327 (comment), linked to making sure audits with warnings are shown; which is kind of weird given the test contents. I think the connection (in my head?) was that we previously marked audits with warnings as failures to make sure they showed up by default, but that all changed when we added groups for all categories in #4278.

However the conversation at the time was about the "X failed audits" header, which might be the "header" mentioned in the TODO. But that would have required a very simple adjustment of the selector to fix.

Lessons:

  • don't leave it.skip() tests for very long because you're going to forget why you skipped
  • make sure to document the results of "discussing in the next meeting" in the PR/issue because otherwise all reasoning will be lost in the mists of time ⏳💨

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM


// All the group names in the config.
const groupNames = Array.from(new Set(auditRefs.map(ref => ref.group))).filter(Boolean);
assert.ok(groupNames.length > 5); // Make sure there are groups to test.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could turn the comment into an assertion message just to make things easier when it fails. though jest comes to our aid here pretty nicely already :)


for (const groupName of groupNames) {
const groupAuditRefs = auditRefs.filter(ref => ref.group === groupName);
assert.ok(groupAuditRefs.length > 0); // Make sure there are audits to find.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

@brendankenny
Copy link
Member Author

good call

@paulirish paulirish merged commit 120d769 into master Jan 14, 2019
@paulirish paulirish deleted the failedtest branch January 14, 2019 23:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants